Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve time precision on decode #676

Closed
wants to merge 1 commit into from
Closed

Conversation

thenrio
Copy link
Contributor

@thenrio thenrio commented May 15, 2024

Fixes #675.

@wojtekmach
Copy link
Member

Thank you for the PR. I honestly don't know how we should proceed. It's a real shame that pg does not send the precision over the wire.

I created a little test rig below:

Mix.install([
  {:ecto_sql, "~> 3.10"},
  {:postgrex, ">= 0.0.0", github: "elixir-ecto/postgrex", override: true}
  # {:postgrex, ">= 0.0.0", github: "thenrio/postgrex", branch: "time", override: true}
])

Application.put_env(:foo, Repo, database: "mix_install_examples")

defmodule Repo do
  use Ecto.Repo,
    adapter: Ecto.Adapters.Postgres,
    otp_app: :foo
end

defmodule Main do
  import Ecto.Query, warn: false

  def main do
    {:ok, _} = Supervisor.start_link([Repo], strategy: :one_for_one)

    Repo.checkout(fn ->
      Repo.query!("CREATE TEMPORARY TABLE times (time time(6))")
      Repo.query!("INSERT INTO times VALUES ('00:00:00'), ('00:00:00.000'), ('00:00:00.123')")
      IO.inspect(Repo.query!("SELECT * FROM times").rows)
    end)
  end
end

Main.main()

and we have:

# main:
[[~T[00:00:00.000000]], [~T[00:00:00.000000]], [~T[00:00:00.123000]]]

# this branch:
[[~T[00:00:00]], [~T[00:00:00]], [~T[00:00:00.123000]]]

I don't have a great mental model for this but I feel like if we standardise on microseconds precision like today at least it is consistent.

@wojtekmach
Copy link
Member

Maybe it doesn't matter, e.g. Ecto distinguishes between :time and :time_usec types and upscales/downscales accordingly anyway.

@josevalim
Copy link
Member

If it is impossible for us to know, then I would prefer to keep the current behaviour. This is because someone may actually store 1:00:00.000000 and we should not render it back as 1:00:00.

@greg-rychlewski
Copy link
Member

postgres lets you specify the precision in the type. something like timestamp p where p is the precision of the fraction. it's been a while since i read the postgrex code but if i was going to try, i would see if we could somehow use the type information. i feel like postgres gives it to us in one of the protocol messages, but don't know how accessible it is.

@wojtekmach
Copy link
Member

Looking at Postgrex.Extensions.Time there is no indicator on the wire so we can’t do much and need to settle one way or another.

@josevalim
Copy link
Member

I think @wojtekmach is correct. The extension is generic for any time precision. We don't have the information for a given time. :(

@thenrio
Copy link
Contributor Author

thenrio commented May 16, 2024

Indeed, you are right @wojtekmach.

May be we can improve documentation, with examples?


For the record, I ran the postgres jdbc driver, with log turned on, for both execute of the prepared statements:

"select now()::time"
"select now()::time(0)"

Both yield the same logging

[2024-05-16 11:28:19] [FINEST ]  FE=> Bind(stmt=null,portal=null) 
[2024-05-16 11:28:19] [FINEST ]  FE=> Describe(portal=null) 
[2024-05-16 11:28:19] [FINEST ]  FE=> Execute(portal=null,limit=0) 
[2024-05-16 11:28:19] [FINEST ]  FE=> Sync 
[2024-05-16 11:28:19] [FINEST ]  <=BE ParseComplete [null] 
[2024-05-16 11:28:19] [FINEST ]  <=BE BindComplete [unnamed] 
[2024-05-16 11:28:19] [FINEST ]  <=BE RowDescription(1) 
[2024-05-16 11:28:19] [FINEST ]         Field(now,TIME,8,T) 
[2024-05-16 11:28:19] [FINEST ]  <=BE DataRow(len=8) 
[2024-05-16 11:28:19] [FINEST ]  <=BE CommandStatus(SELECT 1) 
[2024-05-16 11:28:19] [FINEST ]  <=BE ReadyForQuery(I) 

In both case, the "type" of first column is the same "Field(now,TIME,8,T) "...

@greg-rychlewski
Copy link
Member

greg-rychlewski commented May 16, 2024

There is an indicator in the row description message though. If I print out the name and type_mod fields here I get the following:

query("CREATE TABLE test (micro timestamp, milli timestamp(3))", [])
query("SELECT * FROM test", [])
{"micro", -1}
{"milli", 3}

This is probably how other people would know the correct precision. So what I mean is if we can somehow change the code to utilize it that would be the "correct" way.

@thenrio
Copy link
Contributor Author

thenrio commented May 16, 2024

Indeed, throwing a dgb in the code shows that (far better than my java experiment!).

iex(4)> Postgrex.query!(pid, "select now()::time", [])
[(postgrex 0.17.5) lib/postgrex/messages.ex:431: Postgrex.Messages.decode_row_field/1]
field #=> {:row_field, "now", 0, 0, 1083, 8, -1, 0}

%Postgrex.Result{
  command: :select,
  columns: ["now"],
  rows: [[~T[12:23:56.675846]]],
  num_rows: 1,
  connection_id: 17480,
  messages: []
}
iex(5)> Postgrex.query!(pid, "select now()::time(0)", [])
[(postgrex 0.17.5) lib/postgrex/messages.ex:431: Postgrex.Messages.decode_row_field/1]
field #=> {:row_field, "now", 0, 0, 1083, 8, 0, 0}

%Postgrex.Result{
  command: :select,
  columns: ["now"],
  rows: [[~T[12:24:14]]],
  num_rows: 1,
  connection_id: 17480,
  messages: []
}

We have a type_mod of -1 for now()::time, and 0 for now()::time(0), where 0 is the precision in later expression.
I'll have a look at that, thanks @greg-rychlewski !

@thenrio
Copy link
Contributor Author

thenrio commented May 16, 2024

A go and see shows that we we do not keep the type_mod in the prepared query:

{:ok, q} = Postgrex.prepare(pid, "", "select now()::time(0)")
{:ok,
 %Postgrex.Query{
   ref: #Reference<0.1696994137.2359820289.212894>,
   name: "",
   statement: "select now()::time(0)",
   param_oids: [],
   param_formats: [],
   param_types: [],
   columns: ["now"],
   result_oids: [1083],
   result_formats: [:binary],
   result_types: [Postgrex.Extensions.Time],
   types: {Postgrex.DefaultTypes, #Reference<0.1696994137.2359951361.207048>},
   cache: :reference
 }}

From row_field record {now", 0, 0, 1083, 8, 0, 0}, we keep the oid (1083), find the decoder (Postgrex.Extensions.Time) but throw away the type_mod=precision (0).

So a plan of changes could be:

  1. store the precision in the prepared query

  2. use the precision when we parse the rows

case rows_recv(s, types, rows, buffer) do

case Types.decode_rows(buffer, result_types, rows, types) do

Now I do not understand Types.decode_rows/4 defined at

Module.create(module, quoted, Macro.Env.location(__ENV__))
yet : there are too many macros for me 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserve time precision when we decode a time
4 participants